Skip to content

Conversation

kou
Copy link
Member

@kou kou commented May 11, 2023

What was the end-user or developer problem that led to this PR?

We can install RubyGems plugin by "gem install XXX". The installed plugin is used from the NEXT "gem ...".

For example, "gem install gem-src kaminari" doesn't use gem-src plugin for kaminari. "gem install gem-src && gem install kaminari" uses gem-src plugin for kaminari.

What is your fix for the problem, implemented in this PR?

How about loading a plugin immediately when the plugin is installed? If this proposal is implemented, "gem install gem-src kaminari" works like "gem install gem-src && gem install kaminari".

Make sure the following tasks are checked

@kou kou force-pushed the plugin-immediately branch 4 times, most recently from 56b0a1e to ef48b92 Compare May 15, 2023 00:47
@kou
Copy link
Member Author

kou commented May 15, 2023

Rebased.

@deivid-rodriguez
Copy link
Contributor

If I have version X of plugin gem-src installed, then that will be loaded right before gem install gem-src:X+1 kaminari runs, but also version X+1 will be loaded right after gem-src is installed, correct? Won't that cause two different versions of the same gem to be loaded? That sounds not good.

@kou
Copy link
Member Author

kou commented May 16, 2023

If I have version X of plugin gem-src installed, then that will be loaded right before gem install gem-src:X+1 kaminari runs, but also version X+1 will be loaded right after gem-src is installed, correct?

Correct.

Won't that cause two different versions of the same gem to be loaded?

Yes. Two different versions of the same gem are loaded in the situation.

That sounds not good.

I agree with you.

How about preventing loading gem-src:X+1 in the situation? gem install gem-src:X+1 kaminari loads gem-src:X before it's executed and gem-src:X+1 isn't loaded while gem install gem-src:X+1 kaminari is executing.
gem-src:X+1 is used (and gem-src:X isn't used) from the next gem install ....

Is this behavior acceptable?

@kou kou force-pushed the plugin-immediately branch from ef48b92 to 6f76759 Compare May 16, 2023 20:57
@kou
Copy link
Member Author

kou commented May 16, 2023

I've implemented the behavior.

What do you think about this?

@deivid-rodriguez
Copy link
Contributor

@kou I'm fine with the updated behavior, thanks. I will review the PR in a few days, just to give other maintainers time to leave their feedback or raise their objections (if any).

@kou
Copy link
Member Author

kou commented May 17, 2023

Thanks!

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seem to be some error output when running tests now, can you have a look? Could those point to some bad side effect on real usage?

/home/runner/work/rubygems/rubygems/tmp/test_rubygems_20230516-2057-9o50ny/gemhome/gems/a-2/lib/rubygems_plugin.rb
Error loading RubyGems plugin "/home/runner/work/rubygems/rubygems/tmp/test_rubygems_20230516-2057-gehefu/gemhome/plugins/a_plugin.rb": cannot load such file --  (LoadError)
Error loading RubyGems plugin "/home/runner/work/rubygems/rubygems/tmp/test_rubygems_20230516-2057-oovia/gemhome/plugins/a_plugin.rb": cannot load such file --  (LoadError)
Error loading RubyGems plugin "/home/runner/work/rubygems/rubygems/tmp/test_rubygems_20230516-2057-rpqs7e/gemhome/plugins/a_plugin.rb": cannot load such file --  (LoadError)


run_post_install_hooks

load_plugin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If run_post_install_hooks happened before load_plugin, then any post install hooks potentially registered by the gem would be run immediately too, right?

For example, say the rdoc gem installs a plugin that generates documentation after gems are installed, then gem install rdoc would immediately generate docs for rdoc itself.

Did you consider this? Do you think it would be desirable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry. I didn't consider this.

rdoc (and yard) use done_installing hook not post_install hook but I can understand what you want to say.

I'll move load_plugin BEFORE run_post_install_hooks.

Copy link
Member Author

@kou kou May 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

BTW, I think that we can remove https://github.com/rubygems/rubygems/blob/master/lib/rubygems/rdoc.rb and implement this by RubyGems plugin in RDoc after we merge this pull request.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I'm not fully sure which behavior is best, but I guess running hooks for the gem right after installing is inline with the spirit of this PR.

Regarding RDoc, I think that's what was proposed by #6021, and it makes sense to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't notice #6021.
I'll work on it after we merge this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation: ruby/rdoc#1171

@kou kou force-pushed the plugin-immediately branch 2 times, most recently from db51e60 to fd454f7 Compare May 25, 2023 05:36
@kou
Copy link
Member Author

kou commented May 25, 2023

There seem to be some error output when running tests now, can you have a look? Could those point to some bad side effect on real usage?

/home/runner/work/rubygems/rubygems/tmp/test_rubygems_20230516-2057-9o50ny/gemhome/gems/a-2/lib/rubygems_plugin.rb
Error loading RubyGems plugin "/home/runner/work/rubygems/rubygems/tmp/test_rubygems_20230516-2057-gehefu/gemhome/plugins/a_plugin.rb": cannot load such file --  (LoadError)
Error loading RubyGems plugin "/home/runner/work/rubygems/rubygems/tmp/test_rubygems_20230516-2057-oovia/gemhome/plugins/a_plugin.rb": cannot load such file --  (LoadError)
Error loading RubyGems plugin "/home/runner/work/rubygems/rubygems/tmp/test_rubygems_20230516-2057-rpqs7e/gemhome/plugins/a_plugin.rb": cannot load such file --  (LoadError)

Sorry. I missed them.
I've fixed them.

Copy link
Contributor

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! This looks good to me, let's hope it doesn't create any bad side effects. But it something unexpected comes up, we can consider it later.

@kou
Copy link
Member Author

kou commented May 25, 2023

Please mention me when we found any unexpected side effect.

We can install RubyGems plugin by "gem install XXX". The installed
plugin is used from the NEXT "gem ...".

For example, "gem install gem-src kaminari" doesn't use gem-src plugin
for kaminari. "gem install gem-src && gem install kaminari" uses
gem-src plugin for kaminari.

How about loading a plugin immediately when the plugin is installed?
If this proposal is implemented, "gem install gem-src kaminari" works
like "gem install gem-src && gem install kaminari".
@kou kou force-pushed the plugin-immediately branch from fd454f7 to 4917d96 Compare May 25, 2023 20:16
@deivid-rodriguez deivid-rodriguez merged commit 09e839c into ruby:master May 25, 2023
@kou kou deleted the plugin-immediately branch May 26, 2023 05:04
deivid-rodriguez added a commit that referenced this pull request Jun 8, 2023
Load plugin immediately

(cherry picked from commit 09e839c)
mterada1228 added a commit to mterada1228/rdoc that referenced this pull request May 27, 2024
Problems

1. If there are braking changes in RDoc, RubyGems is also broken.
2. When we maintain RDoc, we have to change RubyGems.

The reason why they are happened is that RubyGems creates documents about a gem with installing it.

Note that RubyGems uses functions of RDoc to create documents.

Specifically,

- Creating documents is executed by `rubygems/lib/rubygems/rdoc.rb`.
- `::RDoc::RubygemsHook` which is defined by RDoc is called by the file.

Solution

RubyGems has the plugin system.

If a gem includes `rubygems_plugin.rb`, RubyGems loads it.
RubyGems executes a process defined in it while installing gems, uninstalling gems or other events.

We can use the system to solve the problems.

The root cause is RubyGems directly references the class of RDoc.

We can remove the root cause by making RDoc RubyGems plugin.

Alternatively `rubygems_plugin.rb` creates documents about gems.

FAQ

Q1. Do we need to change codes of RubyGems?

A.

No, we don't.

If we change codes of RubyGems,
we can't keep a compatibility.

Example:

If we delete codes that uses `RDoc::RubygemsHook` in `rubygems/lib/rubygems/rdoc.rb`,
documentations are not created with old RDoc.

Q2. When can we delete `rubygems/lib/rubygems/rdoc.rb`?

A.

We can delete it when all users use RDoc including `rubygems_plugin`.

Next ruby version is 3.4.

If it includes the RDoc including `rubygems_plugin`,
we can delete `rubygems/lib/rubygems/rdoc.rb` after ruby 3.3 is EOL.

Q3. Is it a breaking change that Rubygems creates documents with
rubygems_plugin not RDoc::RubygemsHook?

A.

If we simply implement this approach,
we move the implementation from `rdoc/lib/rdoc/rubygems_hook.rb` to
`rubygems_plugin.rb`.

This way can be breaking change.

It seems to be fine that we just need to delete `rdoc/rubygems_hook.rb` but it doesn't work.
It generates multiple documents.

`rubygems/lib/rubygems/rdoc.rb` has the following code.

```
begin
  require "rdoc/rubygems_hook"
  # ...
rescue LoadError
end
```

This code ignores RDoc related processes when `rdoc/rubygems_hook` can't be required.
But, this 'require' is not failed.

This is because Ruby installs Rdoc as a default gem.

So, Rdoc installed as a default gem generates documents and one installed as a normal gem does it too.

If you think that this behavior is accectable,
we can just delete `rdoc/rubygems_hook.rb`.

What do you think about this approach?

In this change, we take another approach to solve the problem that creates multiple documents.

The reason why the approach that just deletes `rdoc/rubygems.rb` creates multiple documents is `Gem.done_installing(&Gem::RDoc.method(:generation_hook))` in `rubygems/rdoc.rb`.

`rubygems/rdoc.rb` が Gem.done_installing しているところで ドキュメント生成がデフォルト gem によって行われているので、既存の Rubygems が require している RDoc::RubygemsHook は何も処理を実行しないようにしている。

↑ 次回:
- 動作確認について、手順、結果

Note

When RDoc and other gem were installed,
RubyGems plugin of RDoc wasn't executed.

For example

```
gem install rdoc pkg-config
```

Expected
	Installing RDoc creates a document of pkg-config.
Actual
	Installed RDoc creates a document of pkg-config.

See also.

ruby/rubygems#6673

| RDoc / rubyGems | New | Old |
| changed         |  A  |  A  |
| current         |  A  |  A  |

Because RDoc is default gem,

インストール済みの RDoc でドキュメント生成される。

ただし、この RDoc のバージョン以降を一度インストールすれば、
以降は Plugin によってドキュメント生成がされる。

旧バージョンとの違いは、
Plugin がドキュメント生成するかどうかの違いで、
アウトプットには違いがない。

=======

テスト結果の説明のために、
rdoc 未インストールの状態で、gem install rdoc pkg-config を実行してみる。
このパターンでは実行結果が Expect になるはず。

=> Ruby 3.x 以降で RDoc が default gem から bundled gem になる提案がある。
   将来の話、というテイでテストしておくのもあり?
=> 実際にテストしてみたところ、
   installing の rdoc で plugin を利用して document 生成がされた。

テスト方法

default gem がない状態を再現する方法

1. Move to a directory includes default gems.

$ cd $(ruby -e 'print RbConfig::CONFIG["rubylibdir"]')

2. Rename 'rdoc' and 'rdoc.rb'.

$ mv rdoc{,.bk}
$ mv rdoc.rb{,.bk}

3. Move to a directory includes specifications of default gems.

$ cd $(gem environment gemdir)/specifications/default

4. Rename 'rdoc-<versions>.gemspec'.

$ mv $(echo rdoc-*.gemspec){,.bk}
kou added a commit to ranguba/rroonga that referenced this pull request Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants